Skip to content

Removes a smattering of, apparent, dead code #10091

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 16, 2025
Merged

Removes a smattering of, apparent, dead code #10091

merged 1 commit into from
Apr 16, 2025

Conversation

telser
Copy link
Collaborator

@telser telser commented Jun 9, 2024 •
edited
Loading

Done during Zurihac:

Using weeder to find unused definitions. The roots, per weeder, should be all of the executables built from the top level directory. So all of the items removed here would appear to not be used in the execution or test paths of the codebase. There are a great many more items flagged by weeder, but this was an attempt to be relatively conservative in the removal.

This focuses the removal on internal facing items only. Which limits the gain, but also the scope.

  • Patches conform to the coding conventions.
  • Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

@telser telser force-pushed the telser/remove-smattering-deadcode branch 4 times, most recently from f009c2e to f06457e Compare June 10, 2024 11:17
@telser telser marked this pull request as ready for review June 10, 2024 12:07
@andreabedini andreabedini force-pushed the telser/remove-smattering-deadcode branch from f06457e to 0659052 Compare June 11, 2024 10:53
@andreabedini
Copy link
Collaborator

Rebasing to include cb1b60f

@telser
Copy link
Collaborator Author

telser commented Jun 13, 2024

Thanks @andreabedini!

@grayjay
Copy link
Collaborator

grayjay commented Jun 13, 2024

Do you happen to still have the changes to cabal-install-solver from the draft? They looked like safe changes.

@telser
Copy link
Collaborator Author

telser commented Jun 13, 2024

Do you happen to still have the changes to cabal-install-solver from the draft? They looked like safe changes.

I do, along with some more that should be safe for cabal-install. The idea was to open that as a follow-on to this.

@andreabedini
Copy link
Collaborator

andreabedini commented Jun 17, 2024 •
edited
Loading

I do, along with some more that should be safe for cabal-install. The idea was to open that as a follow-on to this.

What caused the difference between f009c2e and f06457e? Did you decide to start with some more conservative filter?

@Mikolaj
Copy link
Member

Mikolaj commented Aug 22, 2024

@telser: ping! Any final touches from you before we start the review? Any final comments (or answers to comments)?

@telser
Copy link
Collaborator Author

telser commented Aug 23, 2024

I do, along with some more that should be safe for cabal-install. The idea was to open that as a follow-on to this.

What caused the difference between f009c2e and f06457e? Did you decide to start with some more conservative filter?

Being more conservative at the beginning was a suggestion of someone at Zurihac, though now I'm struggling to recall exactly who that was. Don't want to assign credit/blame without being sure.

@telser: ping! Any final touches from you before we start the review? Any final comments (or answers to comments)?

Thanks bringing this back to mind @Mikolaj! Per the above, I'm happy to go with this as-is or if the consensus to to be more aggressive upfront then amending to be somewhere between what is open and the draft is completely fine by me.

ulysses4ever
ulysses4ever approved these changes Sep 8, 2024
View reviewed changes
Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Sorry it took so long...

I have only one question above.

@gbaz
Copy link
Collaborator

gbaz commented Oct 10, 2024

bump on the above question -- i would like to approve and merge but this should be resolved.

@Mikolaj
Copy link
Member

Mikolaj commented Dec 5, 2024

Ping, ping?

@telser
Copy link
Collaborator Author

telser commented Dec 8, 2024

bump on the above question -- i would like to approve and merge but this should be resolved.

Ping, ping?

Thank you for the ping and apologies for taking so long to get back to this! I believe I've answered the question above, but need some guidance on direction that others are comfortable with.

@Kleidukos
Copy link
Member

@telser Do you have all the guidance you need at this point? :)

@telser telser force-pushed the telser/remove-smattering-deadcode branch from 0659052 to 68153bb Compare January 2, 2025 00:49
@telser
Copy link
Collaborator Author

telser commented Jan 2, 2025

@telser Do you have all the guidance you need at this point? :)

I believe so, thanks!

Have restored the above mentioned test and rebased against master.

@Kleidukos
Copy link
Member

@telser Terribly sorry, but the PR has got a conflict. Please ping me when you resolve it, and we'll do the merging. Thanks!

@Mikolaj
Copy link
Member

Mikolaj commented Apr 10, 2025

ping, ping, ping?

@telser telser force-pushed the telser/remove-smattering-deadcode branch from 68153bb to b4e4ed5 Compare April 13, 2025 00:23
@telser
Copy link
Collaborator Author

telser commented Apr 13, 2025

@telser Terribly sorry, but the PR has got a conflict. Please ping me when you resolve it, and we'll do the merging. Thanks!

@Kleidukos I've rebased and resolved the issues, sorry again for the delays!

ping, ping, ping?

Thanks for the attention grab @Mikolaj!

@telser telser force-pushed the telser/remove-smattering-deadcode branch from b4e4ed5 to 0670044 Compare April 13, 2025 00:29
Mikolaj
Mikolaj approved these changes Apr 14, 2025
View reviewed changes
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

Let me rebase to see if CI is green.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 14, 2025

@mergify rebase

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 14, 2025

rebase

✅ Branch has been successfully rebased

@Mikolaj Mikolaj force-pushed the telser/remove-smattering-deadcode branch from 0670044 to 0c6c173 Compare April 14, 2025 10:00
@Mikolaj
Copy link
Member

Mikolaj commented Apr 14, 2025

We've got an error in "Validate old ghcs 8.0.2". Let me restart the job in case it's a fluke.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 14, 2025

OK, it was a fluke. @telser: feel free to set the merge label (if you accept our invitation to the exclusively club of the people that can do so). BTW, any other code-removing commits are very welcome!

@telser telser added the merge me Tell Mergify Bot to merge label Apr 14, 2025
@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Apr 14, 2025
@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 16, 2025

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 16, 2025

@mergify rebase

@telser @Mikolaj
Removes a smattering of, apparent, dead code.
47d5634 
Using weeder to find unused definitions. There are a great many more,
but this was an attempt to be relatively conservative in the removal.
@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 16, 2025

rebase

✅ Branch has been successfully rebased

@Mikolaj Mikolaj force-pushed the telser/remove-smattering-deadcode branch from 0c6c173 to 47d5634 Compare April 16, 2025 19:56
@mergify mergify bot merged commit 04ab16c into haskell:master Apr 16, 2025
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ulysses4ever ulysses4ever ulysses4ever approved these changes

@Mikolaj Mikolaj Mikolaj approved these changes

@andreabedini andreabedini Awaiting requested review from andreabedini

Assignees
No one assigned
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@telser @andreabedini @grayjay @Mikolaj @gbaz @Kleidukos @ulysses4ever @geekosaur